-
Notifications
You must be signed in to change notification settings - Fork 3
Feat: JSON #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: JSON #141
Conversation
WalkthroughAdds JSON-based migration support and tests. composer.json gains the halaxa/json-machine dependency. New destination at src/Migration/Destinations/JSON.php streams resources to a JSON file with temporary buffering, allowed-column filtering, per-item encoding error handling, and transfer/cleanup logic. New source at src/Migration/Sources/JSON.php streams JSON input using JsonMachine, validates permissions, converts items to Row objects, batches exports, and handles local download. Tests: CSV permission format adjusted; new tests at tests/Migration/Unit/General/JSONTest.php exercise JSON export scenarios. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/JSON.php`:
- Around line 173-211: shutdown() currently throws when the source file doesn't
exist, causing failures for empty exports; instead, detect missing $sourcePath
in the shutdown() method and create an empty JSON array file (write "[]") at
$sourcePath (using $this->local and fopen/fwrite or the existing write helpers),
set $this->jsonStarted appropriately, then continue with the transfer logic to
$destPath using $this->local->transfer and the existing device checks
(referencing shutdown(), $this->outputFile, $this->local, $this->deviceForFiles,
$this->directory, $this->jsonStarted, $this->resourceId).
In `@src/Migration/Sources/JSON.php`:
- Around line 123-125: Validate the format of $this->resourceId before
destructuring so you don't create invalid Database/Table objects: check that
$this->resourceId contains a single ':' (e.g. with strpos or explode and count)
and if not throw a clear InvalidArgumentException (include the offending
$this->resourceId in the message). Only proceed to create new Database(...) and
new Table(...) when validation passes; update the code around the [$databaseId,
$tableId] = explode(':', $this->resourceId) line in JSON.php to perform this
check and throw the exception.
- Around line 112-115: The finally block currently unconditionally calls
$this->device->delete($this->filePath) which can remove the user's source file;
change it to only delete the temporary local copy created by downloadToLocal():
track when a download occurred (use the existing $this->downloaded flag) and
delete from the local device used for download instead of $this->device (either
store the Device\Local instance created in downloadToLocal() as a new property
like $this->localDevice or instantiate a local device (Device\Local('/')) in the
finally block) and call $this->localDevice->delete($this->filePath) only when
$this->downloaded is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/JSON.php`:
- Around line 71-74: Add a PHPMD suppression for the unused parameters in the
JSON destination's report() method: update the docblock for the method
JSON::report(array $resources = [], array $resourceIds = []): array to include a
`@SuppressWarnings`("PHPMD.UnusedFormalParameter") (or equivalent PHPMD
annotation) so the unused $resources and $resourceIds do not trigger PHPMD,
keeping the exact signature intact for named-argument compatibility; ensure the
annotation sits in the method-level docblock immediately above the report()
declaration.
d335caf to
91cb1f2
Compare
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.